-
Notifications
You must be signed in to change notification settings - Fork 9
[BREAKING] Modify naming and numbering of status codes #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
0cc10fa to
4872c83
Compare
62bf9e2 to
5091087
Compare
5091087 to
f9507c6
Compare
service/sidecar/mapping.go
Outdated
| if !IsStatusStoredInDB(status) { | ||
| err := b.withStatus.setFinalStatus(txNum, status) | ||
| if err != nil { | ||
| // This should never happen unless there is a bug in the mapper. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the bug results in a deterministic handling across various instances, it is not a problem. Otherwise, a fork can occur. Hence, why not terminate the sidecar when a bug is identified as it can be serious. Otherwise, we can remove the following lines:
if b.txStatus[txNum] != statusNotYetValidated {
// This can never occur unless there is a bug in the relay or the coordinator.
return errors.Newf("two results for a TX [blockNum: %d, txNum: %d]", b.block.Header.Number, txNum)
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I agree we should terminate on a bug. We should terminate here as well.
The issue is having to propagate the error only for a case that should never happen.
Will it be acceptable to panic in such a case? Using logger.Fatal()? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I am fine with a panic here though the effective go recommendation is to propagate the error to the main. At least, we should print the stack trace here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with propagation to avoid panicking on tests.
service/sidecar/mapping.go
Outdated
| b.rejectTx(msgIndex, hdr, protoblocktx.Status_MALFORMED_BAD_ENVELOPE, envErr.Error()) | ||
| return | ||
| } | ||
| if hdr.TxId == "" { | ||
| b.rejectTx(msgIndex, hdr, protoblocktx.Status_MALFORMED_MISSING_TX_ID, "no TX ID") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we directly set the final status here itself rather than calling rejectTx(). Otherwise, it is possible to callhdr.TxID on nil. Hence, I feel this is bit fragile. In fact, the debug() in IsStatusStoredInDB is accessing hdr.TxID and can result in panic if debug log is enabled. If you want to retain the call to rejectTx(), there has to be some validation on the func parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we directly set the final status here itself rather than calling rejectTx().
The reasoning for using rejectTx() instead of directly setting the final status is to relieve the developer from the burden of understanding when we call "reject" and when we set the status directly.
Having this logic inside rejectTx() makes the reasoning self-explanatory.
it is possible to callhdr.TxID on nil
When data, hdr, envErr := serialization.UnwrapEnvelope(msg) returns without an error, hdr is never nil. So the error you refer to cannot happen.
I feel this is bit fragile. In fact, the debug() in IsStatusStoredInDB is accessing hdr.TxID and can result in panic if debug log is enabled. If you want to retain the call to rejectTx(), there has to be some validation on the func parameters.
debugTx() handles the case of nil header.
The only "fragile" case is when we reject with MALFORMED_BAD_ENVELOPE. In such a case, hdr==nil, and we rely on the fact that this status is not stored in the DB.
To make the code more robust, I added the rejectNonDBStatusTx() method and used it when the status is known in advance and it is not stored in the DB. This also eliminates the recursion.
cendhu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, it looks good to me. A few minor issues need to be addressed.
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
Signed-off-by: Liran Funaro <liran.funaro@gmail.com>
c2f5f0c to
4d811a9
Compare
cendhu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Type of change
Description
preProcessBlockAndSendToCoordinator()fail.preProcessBlockAndSendToCoordinator()topreProcessBlock()andsendBlocksToCoordinator()to have a shared context with all GoroutinesRelated issues